Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Export JsValue from js-sys #3466

Merged
merged 5 commits into from
Jun 25, 2023
Merged

Conversation

kajacx
Copy link
Contributor

@kajacx kajacx commented Jun 6, 2023

This way, you can name the JsValue type without adding a dependency on wasm-bindgen.

On the other hand js-sys already depends on wasm-bindgen, so it's not that bad. Still, returning a type (or requiring it as an argument) that your crate doesn't provide simply feels wrong.

@daxpedda
Copy link
Collaborator

daxpedda commented Jun 6, 2023

I don't think I would mind that.
It would probably make sense to re-export wasm-bindgen itself as well.

Could you add this to the changelog as well?

@kajacx
Copy link
Contributor Author

kajacx commented Jun 6, 2023

@daxpedda Yea, I think re-exporting entire wasm-bindgen crate makes slightly more sence - you will know for sure that the JsValue value type comes from the wasm-bindgen crate because the include path will be js_sys::wasm_bindgen::JsValue, and it will also re-export other types that the js-sys might be using in it's public API.

The downside is that unrelated parts of wasm-bindgen are exported as well (for example, you can annotate your methods with a #[js_sys::wasm_bindgen::wasm_bindgen] macro, which makes little sence, but going with a fine comb to reexport only the types used seems like a lot of work.

Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The downside is that unrelated parts of wasm-bindgen are exported as well (for example, you can annotate your methods with a #[js_sys::wasm_bindgen::wasm_bindgen] macro, which makes little sence, but going with a fine comb to reexport only the types used seems like a lot of work.

Reexports like this are just to make it easier to use dependencies without having to pull in sub-dependencies. So it's fine like this, it's a known pattern in Rust.

This is an opinionated change anyway, so we will wait for another maintainer to chime in.

crates/js-sys/CHANGELOG.md Outdated Show resolved Hide resolved
crates/js-sys/src/lib.rs Show resolved Hide resolved
crates/js-sys/src/lib.rs Show resolved Hide resolved
@daxpedda daxpedda self-assigned this Jun 6, 2023
@daxpedda daxpedda added the needs review Needs a review by a maintainer. label Jun 6, 2023
@kajacx kajacx force-pushed the export-js-value-from-js-sys branch from 453f280 to c6ef799 Compare June 6, 2023 16:53
@ranile
Copy link
Collaborator

ranile commented Jun 24, 2023

If this is done, I don't think it should be done in just js-sys. Why not web-sys as well?

@daxpedda
Copy link
Collaborator

I agree. WDYT overall though?

@ranile
Copy link
Collaborator

ranile commented Jun 24, 2023

WDYT overall though?

I'm not a fan of it. All it saves is one line in Cargo.toml. But I also can't think of a reason not to add it

@daxpedda
Copy link
Collaborator

It's generally considered good behavior to re-export crates that you use types of in your public API, which is the case for us in js-sys and web-sys.

But I'm happy to proceed now @kajacx if you finish this up.

Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also re-export wasm-bindgen and js-sys from web-sys.

crates/js-sys/CHANGELOG.md Outdated Show resolved Hide resolved
@daxpedda daxpedda added waiting for author Waiting for author to respond and removed needs review Needs a review by a maintainer. labels Jun 24, 2023
@kajacx
Copy link
Contributor Author

kajacx commented Jun 24, 2023

All it saves is one line in Cargo.toml

Yea, but tracking down that line is non-trivial. You have to go to the source of js-sys or find it on crates.io to get the version of wasm-bindgen it is using. And then you have to keep it updated as js-sys and it's wasm-bindgen dependency gets updated.

If you could use js_sys::wasm_bindgen instead, then you would have a 100% guarantee that you are using the same version of wasm_bindgen as js_sys is using, without the need to manually update anything.

On the other hand, if someone uses my crate, and they want to use JsValue as well, then they will you have to write my_crate::js_sys::wasm_bindgen::JsValue, which is getting ridiculous.

In the end, this is a preference decision, so I think it's best to keep it to the authors of the crate to make the final call. For example, I will not be exporting js-sys from my crate in this way, since that would defeat it's purpose.

@daxpedda
Copy link
Collaborator

Let's also re-export wasm-bindgen and js-sys from web-sys.

@kajacx this is still missing, otherwise this is good to go.

kajacx and others added 5 commits June 25, 2023 09:59
@kajacx kajacx force-pushed the export-js-value-from-js-sys branch from 5655390 to cc03d6b Compare June 25, 2023 07:59
@kajacx
Copy link
Contributor Author

kajacx commented Jun 25, 2023

Let's also re-export wasm-bindgen and js-sys from web-sys.

@kajacx this is still missing, otherwise this is good to go.

Done.

@daxpedda daxpedda merged commit 5158764 into rustwasm:main Jun 25, 2023
@daxpedda
Copy link
Collaborator

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for author Waiting for author to respond
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants